changed mcp2515 driver to use nonblocking spi by kikass13 · Pull Request #817 · modm-io/modm

kikass13 · 2022-02-01T11:18:25Z

change mcp2515 to use proper modm::can driver api
change mcp2515 to use the nonblocking spi api
- mostly, initialization routines are still blocking but i guess tahts fine
add example
utilize attachConfigurationHandler for switching device spi types
test example on a nucleo ~~or discovery board~~
optional:
- utilize attachConfigurationHandler for switching device clock speeds

rleh

Nice!
Thanks for taking care of this!

rleh · 2022-02-01T13:25:40Z

 }

+template <typename SPI, typename CS, typename INT>
+modm::ResumableResult<bool>


Shouldn't update() return modm::ResumableResult<void>?
Because nobody (in the main loop) will handle the return value...

it's just the return value of sendMessage indicating if a message was sent this cycle ... not really useful but i thought why not :D ... iwill remove it

it is void now

it is void now

No it's not.

rleh · 2022-02-01T22:47:37Z

Another addition: This driver thould use the SpiDevice abstraction and should make use of void attachConfigurationHandler(Spi::ConfigurationHandler handler)!
@kikass13: We need this, because we have multiple MCP2515 and one ADIS16470 connected to the same SPI.

Off-topic discussion: In the case mentioned above the SPI clock has to be limited to 1MHz or 2MHz (ADIS16470 limitation), but for the MCP2515 a higher SPI clock would be desirable. This probably affects many (SPI) drivers, what would be the most elegant way to solve this in modm?

chris-durand · 2022-02-01T23:12:48Z

Off-topic discussion: In the case mentioned above the SPI clock has to be limited to 1MHz or 2MHz (ADIS16470 limitation), but for the MCP2515 a higher SPI clock would be desirable. This probably affects many (SPI) drivers, what would be the most elegant way to solve this in modm?

That requires to reconfigure SPI timings when the SpiDevice acquires the SpiMaster. A method similar to the configuration handler could be used but this time it is provided externally by the user and not the driver.
The handler function pointer could be put into the SpiDevice class. It would be called inside of the SpiMaster implementation. If not used the runtime overhead of the solution would be a not taken branch with a nullptr check inside of the SpiMaster::acquire() method.

chris-durand · 2022-02-01T23:50:13Z

@rleh I have quickly hacked together a sub-optimal implementation for switching the SPI baudrate with multiple devices. This is just a rough proof of concept for STM32 only and makes changes to the SpiMaster API.

https://github.com/chris-durand/modm/tree/wip_spi_user_conf_handler

kikass13 · 2022-02-01T23:51:21Z

Another addition: This driver thould use the SpiDevice abstraction and should make use of void attachConfigurationHandler(Spi::ConfigurationHandler handler)! @kikass13: We need this, because we have multiple MCP2515 and one ADIS16470 connected to the same SPI.

Off-topic discussion: In the case mentioned above the SPI clock has to be limited to 1MHz or 2MHz (ADIS16470 limitation), but for the MCP2515 a higher SPI clock would be desirable. This probably affects many (SPI) drivers, what would be the most elegant way to solve this in modm?

I don't know what that is bit I'll look into it tomorrow (after our work meeting stuff)

chris-durand · 2022-02-01T23:58:35Z

I don't know what that is bit I'll look into it tomorrow (after our work meeting stuff)

The configuration handler is actually quite simple. If you have multiple devices on one bus that require different SPI modes you'll need to reconfigure the mode whenever you run a transfer on the next device. You can define a callback that configures the required parameters and modm will call it automatically when needed. Here is an example.

EDIT: if you mean the SpiDevice with your comment, it's a class that manages shared access to one SpiMaster by many device drivers. You basically inherit your driver from SpiDevice<SpiMaster> and put RF_WAIT_UNTIL(this->acquireMaster()); before you do anything with the SPI and call releaseMaster() when you are done. That way it's made sure only one driver uses the bus at the same time. You can look at basically any other SPI based driver to see how it is used.

kikass13 · 2022-02-02T00:32:57Z

I don't know what that is bit I'll look into it tomorrow (after our work meeting stuff)

The configuration handler is actually quite simple. If you have multiple devices on one bus that require different SPI modes you'll need to reconfigure the mode whenever you run a transfer on the next device. You can define a callback that configures the required parameters and modm will call it automatically when needed. Here is an example.

EDIT: if you mean the SpiDevice with your comment, it's a class that manages shared access to one SpiMaster by many device drivers. You basically inherit your driver from SpiDevice<SpiMaster> and put RF_WAIT_UNTIL(this->acquireMaster()); before you do anything with the SPI and call releaseMaster() when you are done. That way it's made sure only one driver uses the bus at the same time. You can look at basically any other SPI based driver to see how it is used.

Yeah the second one .. you explained what it does quite well, thanks :)

kikass13 · 2022-02-04T10:24:57Z

@rleh I have quickly hacked together a sub-optimal implementation for switching the SPI baudrate with multiple devices. This is just a rough proof of concept for STM32 only and makes changes to the SpiMaster API.

https://github.com/chris-durand/modm/tree/wip_spi_user_conf_handler

@rleh @chris-durand
i tested this and seems to work (one would have to check with an oscilloscope if it actually happened, and I don't have one at home :P)

kikass13 · 2022-02-04T14:55:08Z

SpiDevice test seems to be broken? Or did I do something wrong?

https://github.com/modm-io/modm/runs/5065030473?check_suite_focus=true

chris-durand · 2022-02-04T15:03:01Z

SpiDevice test seems to be broken? Or did I do something wrong?
* https://github.com/modm-io/modm/runs/5065030473?check_suite_focus=true

That is because of my experimental commit that was cherry-picked. It is only a proof of concept for STM32. Remove it and everything will be fine :)

rleh · 2022-02-04T15:37:42Z

I think it would actually make sense to force all SPI drivers to always set all "relevant" SPI options, everything else doesn't work consistently and leads to strange behavior, especially when multiple SPI drivers access SPI in different orders one after the other.

The "relevant" SPI options:

Frequency
DataMode
DataOrder
...?

To enforce this I would suggest to implement these three values (with usable default values) in addition to the configurationHandler in the SpiDevice. The SpiDevice class is able to configure this values very efficiently by a single call to the (protected) initialize() function of the respective SpiMaster, whereas calls like SpiMaster::setDataMode(Mode3); SpiMaster::setDataOrder(MsbFirst); in the configurationHandler might reinitialize the SPI peripheral multiple times.

The configurationHandler is optional and should only be used to configure additional IOs/multiplexers/... .

What do you think @chris-durand?

kikass13 · 2022-02-04T15:55:46Z

I think it would actually make sense to force all SPI drivers to always set all "relevant" SPI options, everything else doesn't work consistently and leads to strange behavior, especially when multiple SPI drivers access SPI in different orders one after the other. ...

Well I dont see why the SpiDevice is currently not a default base class with mandatory (pure virtual) overrides
To me the driver api looks inconsistent in multiple places

everything is static (or somethimes not when utilizing protothreads, as they require a non static class structure),
unclear positions of setable /mostly important driver specific options / clocks )
the ability for a driver to access the bus without a mutex (SpiDevice class)
to name a few things

chris-durand · 2022-02-04T16:37:50Z

I think it would actually make sense to force all SPI drivers to always set all "relevant" SPI options, everything else doesn't work consistently and leads to strange behavior, especially when multiple SPI drivers access SPI in different orders one after the other.

I agree that it makes sense to make it mandatory for the SPI modes. I am not sure how it would work for the frequency. The frequency is a compile time template parameter in the SPI master because we want to verify it at compile-time. We would need a second API that allows runtime values without compile-time checking to make it configurable.

Furthermore the clock configuration could require some other platform specific settings on future platforms. I would prefer to decouple it from the device drivers. I would rather let the user provide a call back for the speed configuration that is called when the SPI device is acquired.

chris-durand · 2022-02-04T16:47:20Z

Well I dont see why the SpiDevice is currently not a default base class with mandatory (pure virtual) overrides.

I think there is no need for runtime polymorphism and dynamic dispatch for that use case. Couldn't we just pass a configuration as a non-type template parameter to the SpiDevice for setting the modes? SPI modes are known statically at compile time.

It's not a real is-a relationship for let's say an MCP2515 and a generic SPI device. I would argue a MCP2515 is not a specific version of an SPI interface to be used as such. SPI is an implementation detail, in its public interface it's a CAN interface.

chris-durand · 2022-02-04T17:10:39Z

To enforce this I would suggest to implement these three values (with usable default values) in addition to the configurationHandler in the SpiDevice. The SpiDevice class is able to configure this values very efficiently by a single call to the (protected) initialize() function of the respective SpiMaster, whereas calls like SpiMaster::setDataMode(Mode3); SpiMaster::setDataOrder(MsbFirst); in the configurationHandler might reinitialize the SPI peripheral multiple times.

Looking at the STM32 SPI driver it is actually the opposite. initialize() disables the device, configures all settings, enables master mode and enables the device again. Changing the data mode can be performed by one single write when no SPI transaction is running. initialize() is less efficient. I am not sure if it is different on other platforms. In that case introducing a new function to set these parameters in one go could make sense.

I would change the SpiDevice to take a struct value as a template argument that contains relevant settings (mode, data order, any more?). The drivers will be forced to define them and drivers would not touch the configuration handler anymore which is left entirely to the users. The configuration handler could also be used to configure the frequency if the use-case requires it.

kikass13 · 2022-02-04T19:26:36Z

I think there is no need for runtime polymorphism and dynamic dispatch for that use case.

static polymorph is a thing :/

chris-durand · 2022-02-04T20:08:48Z

static polymorph is a thing :/

Yes, but it is normally not done using virtual functions. A classic C++ pattern for static polymorphism is for example CRTP. De-virtualization optimizations usually don't work well at -Os and -Og so I would rather not want to rely on virtual functions where no dynamic polymorphism is required.

Which virtual functions are you thinking of when you said SpiDevice should have them?

chris-durand

Thanks again for refactoring this driver!

chris-durand · 2022-02-04T20:37:32Z


 	using namespace mcp2515;

+	while(!this->acquireMaster()){};


This will block forever if the SPI is acquired by another driver. The code that is releasing the SPI won't run with the cooperative resumable function execution model when you busy-wait in this loop. This function needs to be resumable for it to work. Then you can do RF_WAIT_UNTIL(this->acquireMaster());.

When we have gotten rid of resumable functions in favor of fibers everything will be better I assume...

it's in the part i left blocking (initialization) ... the whole init could be changed to be non blocking, but that's probably not important

Maybe create a initializeWithPrescalerBlocking() function that calls RF_CALL_BLOCKING(initializeWithPrescaler(...))?

chris-durand · 2022-02-04T20:48:31Z

 void
 modm::Mcp2515<SPI, CS, INT>::writeRegister(uint8_t address, uint8_t data)
 {
+	while(!this->acquireMaster()){};


Same as above. The program will get stuck here if the SpiMaster has been acquired already.

these functions are called from initialize(), which should probably be blocking (I kept it blocking)

Imagine the following situation: You use the same SpiMaster with another driver and then initialize the Mcp2515 while another transaction is running from the other device. Then the initialize() will deadlock forever. Even if that situation is not that common, it will cause nasty bugs.

If I am not mistaken every other initialization function that writes SPI / I²C in modm drivers is resumable. If something should be forced to execute blocking there is always RF_CALL_BLOCKING().

Yeah I am aware of that limitation .. I am still playing around with other stuff so I will try to get the init stuff properly set up later :) just have to do other things for a while :D

kikass13 · 2022-02-04T21:49:39Z

static polymorph is a thing :/

Yes, but it is normally not done using virtual functions. A classic C++ pattern for static polymorphism is for example CRTP. De-virtualization optimizations usually don't work well at -Os and -Og so I would rather not want to rely on virtual functions where no dynamic polymorphism is required.

Which virtual functions are you thinking of when you said SpiDevice should have them?

either pure virtual or a crtp adapter, where release and acquire master is done in every spi command automatically. you mutex bus access (which is a wise thing to do) . The fact that a driver can decide to NOT do that is really weird to me :D

chris-durand · 2022-02-04T22:11:11Z

either pure virtual or a crtp adapter, where release and acquire master is done in every spi command automatically. you mutex bus access (which is a wise thing to do) . The fact that a driver can decide to NOT do that is really weird to me :D

Ah, now I understand what you would like to do. Enforcing that the lock on the SPI is held while it is used sounds like a good idea.
If I understand correctly, you'd like to acquire the lock automatically at the start of the SPI transfer and release it afterwards? In many drivers one transaction consists of multiple subsequent transfers that can't be split up. How would you deal with that with that concept?

salkinium · 2022-02-04T22:38:55Z

think it would actually make sense to force all SPI drivers to always set all "relevant" SPI options

When I added the config handler, it was intended to be set by the application and NOT the driver, to avoid exactly this issue. The developer knows what other devices are on the same bus and thus knows what settings to change.
There is also the issue that some devices can only operate at lower speeds (mostly I2C related) and will electrically malfunction (something with slew rates and capacity? I'm not an EE…) on higher speeds.
In that case the driver cannot know this and thus setting a higher speed automatically is bad.

If you only have one device on the bus, you don't need the config handler at all, it is really supposed to be only a user config handler, not a driver config handler.
Obviously this needs to be documented better…

salkinium · 2022-02-05T02:35:00Z

Yeah, I'm fine with that too. It's more thought out with the config struct.

kikass13 · 2022-02-06T13:05:23Z

this whole discussion is now geared towwards changing the whole device / driver api ... as I see it that is a necessary step because (as I said earlier) the api seems to be a little messy. But does that mean this has to be done in this specific pr?

my mcp driver in general:

now kept the old static interface of the old implementation which
- is a good thing, because you dont need a reference to the driver to do can stuff ... a little bit like the internal Can_1 implementation
  * sadly means: NO SpiDevice in static functions (because the SpiDevice can only used with inheritance) nor nonblocking initialize, but it allwos this function to be static (which cn be very useful)
now allows the send and receive functions to static as well
now uses a user controlled protothread context to utilize bus access (nonblocking)

does this matter now anymore?
should this stay this way?
should this be scrapped because the api is a mess and has to be redone anyways?
should this be merged, so that part of the driver /the main part) can utilize SpiDevice and nonblocking send/receive functionality for now, until the api is cleaned up ?

rleh · 2022-02-08T15:41:09Z

this whole discussion is now geared towwards changing the whole device / driver api ... as I see it that is a necessary step because (as I said earlier) the api seems to be a little messy. But does that mean this has to be done in this specific pr?

This does not have to happen in this PR, in my view it is even nicer to do it in a separate PR.

chris-durand · 2022-02-08T15:45:22Z

I guess @kikass13 might be asking if we should change the API of the MCP2515 driver, not the SpiDevice. Using SpiDevice requires an instantiated object but the driver had only static methods. Now half of them are static. This is messy. I would be in favor of changing the MCP2515 API to make it more consistent.

kikass13 · 2022-02-08T15:55:05Z

@chris-durand currently i like the fact that this thing is static. I dislike the "mix" but i can live with it ... I have some software running this thing currently (successfully), and I really depend on the fact, that the can interfaces can be used statically.

i.e. just provide it as a template argument and call some functions on it (without pushing / holding) references to everything and so on

At least the current implementation works for me

after fixing some of the random ci errors for avr

rleh · 2022-02-08T16:05:56Z

I really depend on the fact, that the can interfaces can be used statically

Can't you in the class where you passed the static class as a template argument instantiate an object of the template argument? Then it works with both static and non-static classes.

kikass13 · 2022-02-08T16:10:33Z

I really depend on the fact, that the can interfaces can be used statically

Can't you in the class where you passed the static class as a template argument instantiate an object of the template argument? Then it works with both static and non-static classes.

sure, i was just trying to explain that the old "design" could still be valid. @rleh you blocked my initial changes because I changed the API, so changing it back again now seems kind of redundant :D

rleh · 2022-02-24T00:48:04Z

 #ifndef MODM_MCP2515_HPP
 	#error	"Don't include this file directly, use 'mcp2515.hpp' instead!"
 #endif
-


Unnecessary whitespace change.

Not resolved.

rleh · 2022-02-24T00:55:14Z

+			if(not modm_assert_continue_ignore(rxQueue.push(messageBuffer_), "mcp2515.can.tx",
+			"CAN transmit software buffer overflowed!", 1)){
+				/// ignore
+			}


Remove the if statement if the body is empty.

well it was there before ... it shows intent .. so i kept it for now

kikass13 · 2022-02-24T21:23:18Z

@rleh i have changed all the points you made (hopefully to your satisfaction) :)

rleh

Inside initializeWithPrescaler() SPI access is performed without locking the bus (using SpiDevice::acquireMaster()/::releaseMaster())
writeRegister(), readRegister() and bitModify() also perform SPI access without using SpiDevice::acquireMaster()/::releaseMaster() and are not resumable

You have to switch from the update() resumable function to a protothread, and inside the protothread you can then do the initialization stuff, triggered by a flag set from the initialize() function.

rleh · 2022-03-16T14:59:27Z

+    <option name="modm:driver:mcp2515:buffer.tx">32</option>
+    <option name="modm:driver:mcp2515:buffer.rx">32</option>


32 is already the default value. Remove?

not a good idea, this shows intent .. nobody would know that this exists otherwise

Everybody that reads the documentation or uses lbuild discover-options will know about it...

rleh · 2022-03-16T15:04:57Z

+namespace mcp2515{
+namespace options{
+
+%% if options["buffer.tx"] > 0


Suggested change

%% if options["buffer.tx"] > 0

buffer.tx is enforced to be within range 1..(2^16-2) by the lbuild module.

(Same for rx buffer)

rleh · 2022-03-16T15:06:03Z

 #ifndef MODM_MCP2515_HPP
 	#error	"Don't include this file directly, use 'mcp2515.hpp' instead!"
 #endif
-


Not resolved.

rleh · 2022-03-16T15:45:20Z

 }

+template <typename SPI, typename CS, typename INT>
+modm::ResumableResult<bool>


it is void now

No it's not.

rleh · 2022-03-16T15:46:55Z

+template <typename SPI, typename CS, typename INT>
+modm::ResumableResult<bool>
+modm::Mcp2515<SPI, CS, INT>::update(){
+	using namespace mcp2515;
+
+	RF_BEGIN();


Shouldn't that rather be a Protothread and not a resumable function?

dunno what you mean. As the api is right now (static), there exists a protothread in the user code (which you told me in a prior version of this driver update, that it should be consistent to the general driver api) ... which one is it?

Hint:

Suggested change

template <typename SPI, typename CS, typename INT>

modm::ResumableResult<bool>

modm::Mcp2515<SPI, CS, INT>::update(){

using namespace mcp2515;

RF_BEGIN();

template <typename SPI, typename CS, typename INT>

void

modm::Mcp2515<SPI, CS, INT>::run()

{

PT_BEGIN();

//...

then the user hjas two protothreads runnign inside each other?
why should the thread be in the driver instead of the user deciding when or in which context update is called?

Hint:

not making a point clear / answering the implicity question of what do you actually want to do and why does not help me :D

rleh · 2022-03-16T15:47:43Z

-	if ((readStatus(READ_STATUS) & (TXB2CNTRL_TXREQ | TXB1CNTRL_TXREQ | TXB0CNTRL_TXREQ)) ==
+	RF_BEGIN();
+
+	tempS = true;


Meaningful variable name or explanation please...

rleh · 2022-03-16T15:49:10Z

+		ready = false;
 	}
+	return ready;


Suggested change

ready = false;

}

return ready;

return false;

}

return true;

rleh · 2022-03-16T16:11:28Z

 modm::Mcp2515<SPI, CS, INT>::writeRegister(uint8_t address, uint8_t data)
 {
 	chipSelect.reset();
-


Still a lot of unnecessary whitespace change in this part of the file.

rleh · 2022-03-16T16:31:38Z

@kikass13 Please stop marking my change requests which are not resolved as resolved.

kikass13 changed the title ~~changed mcp2515 driver to use rx and tx buffers as well as uphold the…~~ changed mcp2515 driver to use nonblocking spi Feb 1, 2022

chris-durand added enhancement 🌈 example 🔑 labels Feb 1, 2022

rleh reviewed Feb 1, 2022

View reviewed changes

rleh added the advanced 🤯 label Feb 1, 2022

chris-durand reviewed Feb 1, 2022

View reviewed changes

Comment thread src/modm/driver/can/mcp2515_impl.hpp Outdated

kikass13 force-pushed the mcp2515_protothread_update_3 branch from dc57d6f to 03639b4 Compare February 1, 2022 20:26

chris-durand reviewed Feb 1, 2022

View reviewed changes

Comment thread src/modm/driver/can/mcp2515_impl.hpp Outdated

chris-durand reviewed Feb 4, 2022

View reviewed changes

Comment thread examples/nucleo_f439zi/can_m2515/main.cpp Outdated

kikass13 force-pushed the mcp2515_protothread_update_3 branch from 839b40a to f94d31a Compare February 4, 2022 19:30

chris-durand reviewed Feb 4, 2022

View reviewed changes

rleh reviewed Feb 24, 2022

View reviewed changes

Comment thread examples/nucleo_f439zi/can_m2515/main.cpp Outdated

rleh requested changes Feb 24, 2022

View reviewed changes

kikass13 force-pushed the mcp2515_protothread_update_3 branch from 814c603 to 5aa143e Compare February 27, 2022 16:42

rleh requested changes Mar 16, 2022

View reviewed changes

rleh added this to the 2022q1 milestone Mar 16, 2022

rleh mentioned this pull request Mar 28, 2022

2022q1 release #842

Merged

30 tasks

rleh modified the milestones: 2022q1, 2022q2 Mar 30, 2022

rleh modified the milestones: 2022q2, 2022q3 Jul 1, 2022

salkinium removed this from the 2022q3 milestone Oct 1, 2022

rleh added the stale ♾ label Apr 9, 2023

salkinium added 2 commits September 8, 2025 00:50

[can] Make dependency on IOStream optional

53ffb0a

[can] Add a static object wrapper class for CAN

ddbbc56

salkinium force-pushed the mcp2515_protothread_update_3 branch from 0ff3eef to 7959cdc Compare September 7, 2025 22:51

[can] Adapt MCP2515 driver to CAN interface

db9f10e

salkinium force-pushed the mcp2515_protothread_update_3 branch from 7959cdc to 3b3ee65 Compare September 8, 2025 18:53

[examples] Add MCP2515 example

3db3bcd

salkinium force-pushed the mcp2515_protothread_update_3 branch from 3b3ee65 to 3db3bcd Compare September 8, 2025 19:14

salkinium added this to the 2025q3 milestone Sep 8, 2025

salkinium merged commit 3db3bcd into modm-io:develop Sep 8, 2025
12 checks passed

		<option name="modm:driver:mcp2515:buffer.tx">32</option>
		<option name="modm:driver:mcp2515:buffer.rx">32</option>

Conversation

kikass13 commented Feb 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rleh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rleh commented Feb 1, 2022

Uh oh!

chris-durand commented Feb 1, 2022

Uh oh!

chris-durand commented Feb 1, 2022

Uh oh!

kikass13 commented Feb 1, 2022

Uh oh!

chris-durand commented Feb 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kikass13 commented Feb 2, 2022

Uh oh!

kikass13 commented Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kikass13 commented Feb 4, 2022

Uh oh!

Uh oh!

chris-durand commented Feb 4, 2022

Uh oh!

rleh commented Feb 4, 2022

Uh oh!

kikass13 commented Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chris-durand commented Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chris-durand commented Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chris-durand commented Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kikass13 commented Feb 4, 2022

Uh oh!

chris-durand commented Feb 4, 2022

Uh oh!

chris-durand left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chris-durand Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kikass13 commented Feb 1, 2022 •

edited

Loading

chris-durand commented Feb 1, 2022 •

edited

Loading

kikass13 commented Feb 4, 2022 •

edited

Loading

kikass13 commented Feb 4, 2022 •

edited

Loading

chris-durand commented Feb 4, 2022 •

edited

Loading

chris-durand commented Feb 4, 2022 •

edited

Loading

chris-durand commented Feb 4, 2022 •

edited

Loading

chris-durand Feb 4, 2022 •

edited

Loading

salkinium commented Feb 4, 2022 •

edited

Loading